Fix: Foundation for the correction of BIP-32 incompatability issue. #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* One in 128 accounts are not BIP-32 compatible * See upstream: btcsuite/btcwallet#719 * Bug introduced at: btcsuite/btcutil#172 (!!!) This is the first phase of the fix, which should not introduce any incompatabilities, but I'm still going to do some extensive regression testing of this one first before I'll be recommending that you merge it. The .Child code was is in use in the following places in the pktwallet source code, where it now uses the DeriveNonStandard method: pktwallet/waddrmgr/manager.go:1259 pktwallet/waddrmgr/manager.go:1265 pktwallet/waddrmgr/manager.go:1287 pktwallet/waddrmgr/manager.go:1302 pktwallet/waddrmgr/manager.go:1307 pktwallet/waddrmgr/scoped_manager.go:286 pktwallet/waddrmgr/scoped_manager.go:835 pktwallet/waddrmgr/scoped_manager.go:852 pktwallet/waddrmgr/scoped_manager.go:1035 I'm also using the DeriveNonStandard at the following locations in the integration test code (which shouldn't be necessary) - I'm just being highly conservative here. integration/rpctest/memwallet.go:130 integration/rpctest/memwallet.go:342 integration/rpctest/memwallet.go:476 Again, I have NOT changed in this PR the wallet to actually use the corrected and compatible Derive method, as it would a introduce wallet backwards incompatability: "If you were previously using the Child method, know this method is incompatible. The Child method had a BIP-32 standards compatibility issue. You have to check whether any hardened derivations in your derivation path are affected by this issue, via the new IsAffectedByIssue172() method, and migrate the wallet if so. The new and corrected method (Derive) does conform to all the standards. If you need the old behavior, then use DeriveNonStandard()." A future update will use IsAffectedByIssue172() and include wallet migration code, at which time the DeriveNonStandard method can be safely switched over at the above places in the sources to use properly standards-compliant Derive method.
|
Check the wallet seed code, I believe it's versioned. Correct-bip32 can become seed version number 2. |
|
++ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first phase of the fix, which should not introduce any incompatibilities, but I'm still going to do some extensive regression testing of this one first before I'll be recommending that you merge it.
The
Childcode was is in use in the following places in thepktwalletsource code, where it now uses theDeriveNonStandardmethod:I'm also using the
DeriveNonStandardat the following locations in the integration test code (which shouldn't be necessary) - but I'm just being highly conservative here:Again, I have NOT changed, in this PR, the wallet to actually use the corrected and compatible
Derivemethod, to avoid introducing wallet backwards incompatibility:A future PR will update the code, use
IsAffectedByIssue172(), and include wallet migration code, at which time theDeriveNonStandardmethod can be safely switched over at the above places in the sources to use properly standards-compliantDerivemethod.